feat(operator): scaffold DGD placement score status wiring#11057
Open
ashnamehrotra wants to merge 10 commits into
Open
feat(operator): scaffold DGD placement score status wiring#11057ashnamehrotra wants to merge 10 commits into
ashnamehrotra wants to merge 10 commits into
Conversation
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
June 29, 2026 18:20 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
github-actions
Bot
added
feat
external-contribution
Contributor
|
/ok to test b29bb9a |
github-actions
Bot
added
the
deployment::k8s
This comment has been minimized.
This comment has been minimized.
Contributor
WalkthroughAdds ChangesPlacement Score for DynamoGraphDeployment
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/operator/internal/controller/dynamographdeployment_controller.go`:
- Around line 292-300: Reset the placement status fields in
dynamographdeployment_controller.go so stale values do not survive non-Grove or
lookup-failure reconciles. In the reconcile logic around r.isGrovePathway and
getExistingGrovePodCliqueSet, explicitly clear or set
dynamoDeployment.Status.PlacementScore and
dynamoDeployment.Status.PlacementScoreState on the non-Grove branch and when
pcsErr is returned, instead of only updating them after AggregatePlacementScore
succeeds. Use the existing symbols r.isGrovePathway,
getExistingGrovePodCliqueSet, and dynamo.AggregatePlacementScore to place the
fix in the same status-update path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 03ddbb02-5731-442a-8efc-7bfe4bb28257
📒 Files selected for processing (11)
deploy/operator/api/v1alpha1/conversion_field_coverage_test.godeploy/operator/api/v1alpha1/dynamographdeployment_conversion.godeploy/operator/api/v1alpha1/dynamographdeployment_conversion_test.godeploy/operator/api/v1alpha1/dynamographdeployment_types.godeploy/operator/api/v1alpha1/zz_generated.deepcopy.godeploy/operator/api/v1beta1/common.godeploy/operator/api/v1beta1/dynamographdeployment_types.godeploy/operator/api/v1beta1/zz_generated.deepcopy.godeploy/operator/internal/controller/dynamographdeployment_controller.godeploy/operator/internal/dynamo/grove.godeploy/operator/internal/dynamo/grove_test.go
Comment on lines
+292
to
+300
| if r.isGrovePathway(dynamoDeployment) { | ||
| pcs, pcsErr := r.getExistingGrovePodCliqueSet(ctx, dynamoDeployment) | ||
| if pcsErr != nil { | ||
| logger.V(1).Info("Failed to get PodCliqueSet for placement score", "error", pcsErr) | ||
| } else { | ||
| score, state := dynamo.AggregatePlacementScore(pcs) | ||
| dynamoDeployment.Status.PlacementScore = score | ||
| dynamoDeployment.Status.PlacementScoreState = state | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reset placement status on non-Grove and lookup-error paths.
Lines 292-300 only overwrite these fields on the happy Grove path, so a DGD can keep stale status.placementScore* values after Grove is disabled or after a transient getExistingGrovePodCliqueSet failure. Please clear or explicitly set them on every reconcile path instead of leaving the previous status intact.
Suggested fix
- if r.isGrovePathway(dynamoDeployment) {
+ dynamoDeployment.Status.PlacementScore = nil
+ dynamoDeployment.Status.PlacementScoreState = nvidiacomv1beta1.PlacementScoreStateUnsupported
+ if r.isGrovePathway(dynamoDeployment) {
pcs, pcsErr := r.getExistingGrovePodCliqueSet(ctx, dynamoDeployment)
if pcsErr != nil {
logger.V(1).Info("Failed to get PodCliqueSet for placement score", "error", pcsErr)
+ dynamoDeployment.Status.PlacementScoreState = nvidiacomv1beta1.PlacementScoreStateUnknown
} else {
score, state := dynamo.AggregatePlacementScore(pcs)
dynamoDeployment.Status.PlacementScore = score
dynamoDeployment.Status.PlacementScoreState = state
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if r.isGrovePathway(dynamoDeployment) { | |
| pcs, pcsErr := r.getExistingGrovePodCliqueSet(ctx, dynamoDeployment) | |
| if pcsErr != nil { | |
| logger.V(1).Info("Failed to get PodCliqueSet for placement score", "error", pcsErr) | |
| } else { | |
| score, state := dynamo.AggregatePlacementScore(pcs) | |
| dynamoDeployment.Status.PlacementScore = score | |
| dynamoDeployment.Status.PlacementScoreState = state | |
| } | |
| dynamoDeployment.Status.PlacementScore = nil | |
| dynamoDeployment.Status.PlacementScoreState = nvidiacomv1beta1.PlacementScoreStateUnsupported | |
| if r.isGrovePathway(dynamoDeployment) { | |
| pcs, pcsErr := r.getExistingGrovePodCliqueSet(ctx, dynamoDeployment) | |
| if pcsErr != nil { | |
| logger.V(1).Info("Failed to get PodCliqueSet for placement score", "error", pcsErr) | |
| dynamoDeployment.Status.PlacementScoreState = nvidiacomv1beta1.PlacementScoreStateUnknown | |
| } else { | |
| score, state := dynamo.AggregatePlacementScore(pcs) | |
| dynamoDeployment.Status.PlacementScore = score | |
| dynamoDeployment.Status.PlacementScoreState = state | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/operator/internal/controller/dynamographdeployment_controller.go`
around lines 292 - 300, Reset the placement status fields in
dynamographdeployment_controller.go so stale values do not survive non-Grove or
lookup-failure reconciles. In the reconcile logic around r.isGrovePathway and
getExistingGrovePodCliqueSet, explicitly clear or set
dynamoDeployment.Status.PlacementScore and
dynamoDeployment.Status.PlacementScoreState on the non-Grove branch and when
pcsErr is returned, instead of only updating them after AggregatePlacementScore
succeeds. Use the existing symbols r.isGrovePathway,
getExistingGrovePodCliqueSet, and dynamo.AggregatePlacementScore to place the
fix in the same status-update path.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
June 30, 2026 13:40 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test 0c56c9e |
Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
June 30, 2026 16:33 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test de57d0f |
github-actions
Bot
added
the
documentation
sttts
reviewed
Jul 2, 2026
sttts
reviewed
Jul 2, 2026
ashnamehrotra
temporarily deployed
to
external_collaborator
July 9, 2026 20:01 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test a93fe91 |
Resolve conflicts in the DGD controller (keep placement-status write, drop the duplicate reconcile-error check that main removed) and in grove.go (keep AggregatePlacementScore alongside main's shortened comment on specToGroveTopologyConstraint). Regenerate deepcopy, CRDs, api-reference, helm docs, and helm CRD copy to pick up both trees. Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
July 9, 2026 20:10 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test e3b76e7 |
ashnamehrotra
temporarily deployed
to
external_collaborator
July 10, 2026 15:28 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test df4d992 |
Register no-op custom fuzzers for the v1alpha1 and v1beta1 PlacementStatus so the round-trip fuzz filler does not draw random bits for Placement. Placement round-trip is already covered by TestDGD_RoundTrip_Status. Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
force-pushed
the
grove-placement-score
branch
from
July 10, 2026 15:52
df4d992 to
5214cf8
Compare
ashnamehrotra
temporarily deployed
to
external_collaborator
July 10, 2026 15:52 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test 5214cf8 |
sttts
reviewed
Jul 15, 2026
sttts
reviewed
Jul 15, 2026
sttts
reviewed
Jul 15, 2026
Contributor
|
Comparing this PR with DEP #10064, is the intent to land only the placement-status API/scaffolding here and implement the remaining DEP requirements in follow-ups? In particular:
If these are intentionally deferred, could we link the follow-up issues/PRs or make the partial DEP scope explicit in the PR description? |
Contributor
|
Address @sttts P1 on grove.go:300. AggregatePlacementScore returned Unsupported for every non-empty PodGang list, so Reported/Partial and status.placement.score were unreachable until Grove exposes a placement score. Until the DEP ai-dynamo#10064 open question on score source is resolved, land the API shape (types, CRD, conversion, round-trip tests) without wiring the controller. Removed: - dynamo.AggregatePlacementScore and its unit test - controller.updatePlacementStatus and its call site in Reconcile Kept: - PlacementStatus struct with Score + State (v1beta1 + v1alpha1) - Enum with Reported / Partial / Unsupported / Unknown - Score CRD bounds [0, 1] - v1alpha1<->v1beta1 conversion + round-trip test Doc-comment on v1beta1 PlacementStatus notes the schema-only state and points at DEP ai-dynamo#10064. Regenerated CRDs, api-reference, and helm CRD copy. Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
July 16, 2026 14:33 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test 36d2d27 |
Address @sttts on roundtrip_fuzz_test.go:224. Replace the no-op PlacementStatus fuzzers with real randomizers that pick State from the four PlacementScoreState enum values and Score from either nil or a value inside the CRD [0, 1] bounds. Add an oneOfPtr helper alongside oneOf for the sometimes-nil-sometimes-value pattern that optional API pointer fields need. Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
July 16, 2026 15:05 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test effa9fa |
Resolve modify/delete conflict on the operator helm-chart CRD copy (upstream ai-dynamo#11689 stops bundling CRDs in the Helm subchart) by accepting the upstream delete. Regenerate deepcopy, CRDs, api-reference, and helm docs on top of the merged tree. Signed-off-by: ashnamehrotra <ashnamehrotra@gmail.com>
ashnamehrotra
temporarily deployed
to
external_collaborator
July 16, 2026 15:46 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
/ok to test f7eb656 |
julienmancuso
approved these changes
Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
status.placementScorestatus.placementScoreStatev1beta1andv1alpha1, including conversion wiring and conversion field-set acknowledgment.placementScoreunset until Grove exposes a score field in a consumable API shape.Unsupported/NotReported).Validation
GOCACHE=/tmp/dynamo-go-cache go test ./api/v1alpha1 ./api/v1beta1 -count=1GOCACHE=/tmp/dynamo-go-cache go test ./api/v1alpha1 ./internal/dynamo -count=1GOCACHE=/tmp/dynamo-go-cache go build ./...(fromdeploy/operator)Scope Notes
Reported/Partialyet, because current Grove API does not exposeplacementScorewhere Dynamo can read it.Summary by CodeRabbit